Conversation
Member
|
Having a negative flag is my preferred solution as well. |
rbhbokka
marked this pull request as ready for review
September 18, 2026 22:36
Regression from OpenAstroTech#291. DecCoordinate, MeadeLatitude and MeadeLongitude carried the sign in the sign bit of `degrees`, which cannot represent a negative value whose degrees component is zero. Cursor::signed2() computes -(int)0, which is 0, so the sign was destroyed inside the struct before any handler saw it: :Sd-00*30:00# -> {0, 30, 0} sets +00*30:00 :St-00*30# -> {0, 30} equatorial sites :Sg-000*05# -> {0, 5} central London A one-degree error in a band straddling the celestial equator, and :CM sync writes it into the mount's home reference permanently. The pre-OpenAstroTech#291 DayTime::ParseFromMeade applied the sign to the whole total and was correct. Replace signed2/signed3 with Cursor::optionalSign(), which reports the sign without folding it into a magnitude, and give the three structs an explicit `negative` field. The readers keep sign and magnitude apart to the end, and the writers and the MeadeCommandProcessor boundary read the sign off the undivided total rather than off a divided degrees component. The accepted grammar is byte-for-byte unchanged -- readMandatorySign() preserves the existing requirement for an explicit sign, so this commit changes only what the parser does with a sign it already accepted. This supersedes OpenAstroTech#241, which diagnosed the same root cause and proposed the same remedy of carrying the sign as its own channel. Its two target functions, Longitude::formatString() and Longitude::formatStringForMeade(), have had no callers since OpenAstroTech#291 routed around them, so the idea is applied here where the code now lives. Co-authored-by: Claude <noreply@anthropic.com>
The parser keeps sign and magnitude apart, but decFromWire flattened the flag back into a signed `deg` for fromCelestialDegrees, and integer 0 has no sign. ":Sd-00*30:00" and ":Sd+00*30:00" both landed on axis seconds 322200; -00*30:00 is 325800. Exactly one degree, silently, for any target or sync inside the first degree south of the celestial equator. Add core::Declination::celestialSecondsFrom, the declination counterpart of the site join, and Declination::fromCelestialSeconds to consume it, so decFromWire composes the two and holds no arithmetic of its own. The join lives in core because the native test environment builds only src/core, src/ports and src/adapters -- src/MeadeCommandProcessor.cpp and src/Declination.cpp are Arduino-dependent and never compiled there, which is why the parser-level tests could pass while the wire boundary was wrong. The new tests pin both the defective composition and the correct one side by side, in both hemispheres. fromCelestialDegrees keeps its comment block describing the limitation and now has no production caller. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01StH2aGiQEj3qvMWJ58CSWz
rbhbokka
force-pushed
the
fix/meade-sign-of-zero
branch
from
September 19, 2026 02:20
ac9ba5d to
52f7d18
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Regression from #291, and a supersede proposal for #241 — see below. Draft, because it changes three public struct shapes and I'd like agreement on the approach before polishing.
The defect
DecCoordinate,MeadeLatitudeandMeadeLongitudecarry the sign in the sign bit ofdegrees:That cannot represent a negative value whose degrees component is zero.
Cursor::signed2()computes-static_cast<int>(0), which is0, so the sign is destroyed inside the struct, before any handler can see it::Sd-00*30:00#{0, 30, 0}+00*30:00:St-00*30#{0, 30}+00*30— equatorial sites:Sg-000*05#{0, 5}+000*05— central LondonA one-degree error in a band straddling the celestial equator, and
:CMsync takes the same path, so it lands in the mount's home reference permanently rather than in a single slew.The pre-#291
DayTime::ParseFromMeadeapplied the sign to the whole total (sgn * (((degs * 60 + mins) * 60) + secs)) and was correct.The fix
Cursor::optionalSign()replacessigned2/signed3. It reports the sign without folding it into a magnitude, which is the primitive whose absence caused the problem — the old helpers bundled "consume a sign" with "consume N digits", and the sign died in the bundling.negativefield and an unsigned magnitude.MeadeCommandProcessorboundary read the sign off the undivided total rather than off a divided degrees component.The accepted grammar is byte-for-byte unchanged.
readMandatorySign()preserves the existing requirement for an explicit sign, so this commit changes only what the parser does with a sign it already accepted. A differential fuzz over 176,186 inputs across the whole Set grammar shows the sign-of-zero cases as the only behavioural difference; an exhaustive sweep of the Get writers is byte-identical todevelop.On #241
#241 diagnosed this exact root cause in 2024 and proposed the right remedy — carry the sign as its own channel instead of in the sign bit of a magnitude. Its two target functions,
Longitude::formatString()andLongitude::formatStringForMeade(), have had no callers since #291 routed around them, so it no longer changes behaviour. This applies the same idea where the code now lives. Credit to @peteasa for the diagnosis; happy for this to be closed in favour of a reworked #241 if that's preferred.Approach — the thing I'd like ruled on
I took the smaller of two options: an explicit
negativefield. The alternative is replacing the components with a single signed total (int32_tarcseconds / arcminutes) plusfromDms/toDms, which makes the hole impossible rather than merely fixed and collapses several call sites — but it is a much larger diff against structs that #296 and #299 are currently contesting.If you'd rather have the signed-total version, say so and I'll redo it that way. The redundant state in the current shape (a magnitude that can't be negative, plus a flag) is a fair criticism and I'd rather resolve it before this is polished than after.
Verification
pio test -e native: 298 pass (269 ondevelop+ 29 added, none removed). Builds clean under-Werrorforoaeboardv1(ESP32) andramps(AVR, 16-bitint— relevant since the struct layout changed).Not fixed here
Declination::fromCelestialDegrees()has a separate sign bug of its own, fixed in #302. The two are complementary: #302 makes the join move the magnitude away from zero, this one makes sure a sign exists to move it in.File overlap
Rewrites the three readers that #304 (
:Sg) modifies, and touchesMeadeParserSet.cppin common with #303. Independently mergeable; I'll rebase whichever lands later.